feat(prerender): use organic+included urls when audit is triggered from Slack#2307
feat(prerender): use organic+included urls when audit is triggered from Slack#2307ssilare-adobe wants to merge 14 commits intomainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
This PR will trigger a minor release when merged. |
anuj-adobe
left a comment
There was a problem hiding this comment.
Code Review
Overview
When run {site} prerender is triggered via Slack, the prerender audit now detects this via auditContext.slackContext.channelId and uses a simplified strategy: organic + included URLs only, skipping agentic URL sources and daily batching. Scheduled runs are unaffected.
Code Quality & Readability
src/prerender/handler.js
-
Variable pre-declaration block is a readability concern (around the
isSlackTriggeredbranch):let finalUrls; let filteredCount; let agenticUrlsCount = 0; let currentAgentic = 0; let currentOrganic; let currentIncludedUrls; let isFirstRunOfCycle;
Six
letdeclarations before anif/elseis a code smell. The Slack path and scheduled path are divergent enough to warrant extraction into separate helper functions (e.g.,buildSlackTriggeredBatch(...)/buildScheduledBatch(...)), which would eliminate these shared mutable vars and make each path self-contained. -
isNotRecentUrlmoved up — relocated from afterimportTopPagesto just aftergetRecentlyProcessedPathnames. Improves logical proximity. No correctness issue. -
Minor: cleaner
siteId—siteId: site.getId()→siteId(already destructured above). Clean. -
Log message formatting fixed — the multi-line log had a leading newline before
prerender_submit_scraping_metrics:. Good cleanup.
Potential Issues / Regressions
🚨 Critical gap: fallback path in processContentAndGenerateOpportunities not guarded
The spec explicitly states:
Step 3 fallback path: skips agentic fetch when mode === 'organic'
But the diff only changes a comment in step 3 — the actual getTopAgenticUrls() call in the fallback is not gated on isSlackTriggered. If a Slack-triggered run produces zero scrape results, the fallback will still fetch agentic URLs — contradicting the intent. This is a bug.
Removed metric: agenticNewThisCycle
The previous log included agenticNewThisCycle=${filteredAgenticUrls.length} — the count of agentic URLs new in this cycle. This was replaced with isSlackTriggered. If anyone is parsing this log field for monitoring/alerting, it's a silent regression worth noting.
Spec/implementation mismatch
The design doc (2026-04-06-prerender-organic-mode-design.md) describes using MODE_ORGANIC = 'organic' driven by explicit {"mode":"organic"} in the SQS message data (like MODE_AI_ONLY). The actual implementation diverged to use Slack context detection instead. The spec is now stale/misleading and should be updated to reflect the actual approach, or removed.
Test Coverage
Tests added cover:
- ✅ Slack-triggered: organic URLs included even when "recently processed"
- ✅ Slack-triggered: agentic URLs NOT fetched
- ✅ Scheduled: agentic URLs still fetched
Missing test: The fallback path in processContentAndGenerateOpportunities for Slack-triggered runs is not tested (and as noted above, the code doesn't guard it either).
Summary
| Area | Status |
|---|---|
| Correctness | |
| Readability | Acceptable, but pre-declared let block warrants refactoring into helpers |
| Test coverage | Missing fallback path test; matches the gap in the implementation |
| Observability | agenticNewThisCycle metric silently removed |
| Docs | Spec is stale relative to implementation |
Recommendation: Block on the step 3 fallback gap. The rest can be addressed or accepted as follow-up.
|
Thanks for the thorough review @anuj-adobe! Critical fix — step 3 fallback not guarded (addressed in 5659973) The Other points from the review
|
|
8149afe to
b550078
Compare
Summary
run {site} prerenderSlack command triggers a prerender audit, the audit worker now detects this viaauditContext.slackContext.channelIdand uses a simplified URL strategy: organic + included URLs only, with no daily batching and no agentic URL sourcesMODE_ORGANICconstant in favor of Slack context detectiongetRecentlyProcessedPathnamesandisNotRecentUrlhelpers for scheduled runsrun {site} prerendercommand is used as-isTest plan
🤖 Generated with Claude Code